Skip to content

Daemon maintenance skip reasons reach a standing surface (#742) - #759

Open
philcunliffe wants to merge 8 commits into
masterfrom
fix/issue-742
Open

Daemon maintenance skip reasons reach a standing surface (#742)#759
philcunliffe wants to merge 8 commits into
masterfrom
fix/issue-742

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Closes #742.

Correction (the description below was rewritten to match what merges).
Two things moved under this PR while it was open:

  • The design record is LLP 0228,
    not LLP 0224.
    This branch drafted the doc as 0224; master then claimed
    0224 for desktop-setup-second-pass, so it was renumbered. Every @ref in
    the code, every anchor, and LLP 0217's Extended-by: entry say 0228. If you
    are reading an older comment on this thread, read LLP 0228 wherever it says
    LLP 0224.
  • PR One partition's failure no longer ends the maintenance walk (#737) #747 has merged (2026-08-14), and this branch is now based on a
    master that contains it. The original body described One partition's failure no longer ends the maintenance walk (#737) #747 as unmerged, not
    depended on, and proposed a mechanical conflict resolution. That resolution
    is exactly what the merge commits on this branch executed; the section below
    records what was done rather than what was proposed.

A partition maintenance deliberately leaves fragmented (LLP 0217's
compactionIneffective, LLP 0218's compactionAttemptFailed) was stated on
two surfaces you have to go looking for: a hyp query maintain line and a
maintenance.partition span attribute. The daemon, which runs the walk
hourly, awaited maintainCache and dropped the report on the floor. An
operator who never ran the command by hand and did not have tracing on when
the tick ran had no way to find a frozen partition at all.

Design record: LLP 0228 (new), with Extended-by: forward-refs added to
LLP 0217 and LLP 0218. Nothing either of those settled is edited.

The surface, and what was rejected

DaemonStatus.maintenance in status.json, written by the maintenance tick
through the daemon's existing persist(); hyp status lifts it, renders a
maintenance: block, carries it under --json, and raises one
maintenance_partitions_skipped warning diagnostic. Plus one
daemon.maintenance_skipped fileLog line per tick when the count is
nonzero. That is #742's option 3: the log line is the record, the status file
is the discovery.

This reuses LLP 0164's route for LLP 0164's reason. The daemon is the only
process that runs the walk; hyp status activates no plugins and reads no
cache, so a status command that re-derived the answer would be firing a second
maintenance walk (metadata loads, data-file stats) to render a status block.

Rejected:

  • A fileLog line alone (Daemon maintenance skip reasons are visible only in spans, never in a standing daemon surface #742's option 1): a record of a tick, not a
    description of a state. It scrolls away, and it is the same shape of surface
    LLP 0218's context section already calls insufficient for the failing tick.
  • A new field on the maintenance report alone: the caller that throws the
    report away is the entire defect.
  • A new file / new command: the status file already exists, is already
    written on every tick, and is already how hyp status learns daemon-only
    facts.
  • Raising overall to degraded: the daemon is running, capture works,
    and queries answer. This sits with recent_errors and a failed client
    action, not with a missing config.

Nothing about when a partition is compacted moves. The tick reads the report
it already produced and stats nothing: proving a skipped partition is also
still fragmented is exactly the per-tick cost the LLP 0199 baseline gate
exists to avoid, and LLP 0217/0218 both already declined to pay it.

Retention rule and its bound

The last completed tick, whole. Every tick overwrites the snapshot,
including one that skipped nothing (which writes zeros).

Why: both reasons are read off the partition cursor on every tick. They
describe a state that is still true, not an event that happened once. So the
newest tick is the only current answer, and a partition that thaws (a
--force rewrite, new data flushing in, the next writer generation) drops off
the surface by itself, with no expiry rule and nothing to invalidate. That is
the same self-clearing property LLP 0218 built into the report. A last-N
history would go stale against the cursor, would need an eviction rule of its
own, and would duplicate the fileLog, which is already the timestamped
append-only record.

Bounded three ways, so it cannot grow:

  • reasons is a fixed key set: one integer per reason id.
  • partitions (the named ones) is capped at MAX_SKIPPED_PARTITIONS_REPORTED
    = 8, taken in walk order, which is LLP 0199#neediest-first (descending live
    data-file count) - so the named ones are the worst ones and the cap costs no
    sort of its own.
  • skippedTotal is the exact count, so the cap is never a lie; the render
    prints ... and N more (hyp query maintain --dry-run lists them all).

The cap and the label sanitizing are applied on write as well as on read
(review round 1 found the write side had only the cap, which made a sentence in
the LLP untrue; sanitizeLabel(...) now runs on the dataset and partition
labels before they reach status.json, which fixes the daemon log's worst
field for free). The read side re-applies both, for the reason
recentEntrypointsFromSources states in the same file: core reads a file,
must not assume this build wrote it, and everything read is about to be printed
to a terminal.

Secret-safety: each entry carries a dataset name, a partition tuple rendered
as k=v/k=v (the same label hyp query maintain prints), a reason id, and
either the recorded rewrite's data-file count or the ISO timestamp of the
spent attempt. Dataset and partition identifiers plus kernel-side counters.
No row data, no prompts, no credentials, no config values are anywhere on the
path.

Reason vocabulary

The ids are the maintenance.partition span attribute names, verbatim,
which are themselves named after the MaintenancePartitionReport fields:

id source means
compaction_ineffective LLP 0217#record-effectiveness, MaintenancePartitionReport.compactionIneffective, span attr compaction_ineffective this writer already rewrote it and reproduced the same file count
compaction_attempt_failed LLP 0218#report-the-spent-attempt, MaintenancePartitionReport.compactionAttemptFailed, span attr compaction_attempt_failed the one retry the writer generation owed it was spent by a rewrite that threw

One spelling across the trace, the status file, hyp status text, --json,
and the daemon log. No new name was minted.

Deliberately not reasons: plain convergence (LLP 0199#baseline-gate) is the
healthy majority of every cache, so naming it would put most partitions on the
surface and none of the interesting ones; a rebaseline (LLP 0207) is work the
tick did; a partition the tick rewrote ineffectively is a run that ran (it
lands on the next tick's snapshot as a skip, which is when it is standing
state); and #747 / LLP 0220's per-partition failed is this tick's error rather
than a stated reason for attempting nothing, so it stays off this surface and
keeps its own daemon.maintenance_failed line.

What the #747 merge actually did

#747 is in master and this branch is merged up to it. The two hunks that
collided were resolved as proposed:

  • src/core/daemon/runtime.js, runMaintenance: master's
    tracer.startActiveSpan skeleton is kept, with its report.totalFailed
    handling and its partitions_failed / partitions_maintained attributes.
    Inside it, after const report = ..., this PR's three statements are
    reinstated: summarizeMaintenanceSkips(report), persist({ maintenance }),
    and the gated daemon.maintenance_skipped warn line, with
    partitions_visited / partitions_skipped set beside master's attributes.
  • llp/0218, the header: one Extended-by: line carrying both entries
    (LLP 0220 and this doc), separated by ; , per the corpus convention.

The composed behaviour is pinned by a test rather than by argument: a partition this tick failed on is not a skip feeds a #747-shaped report
({ failed: true, errorKind: ... }) through summarizeMaintenanceSkips and
asserts skippedTotal: 0, no text block, and no diagnostic. LLP 0220
#this-tick-versus-a-recorded-one says the same thing from the other side. If a
later request wants this tick's failures standing too, the shape takes it
additively: a third reason id and a third count key, no shape change.

Tests and discrimination evidence

test/core/status-maintenance-skips.test.js, 14 tests at this head (11
originally, plus the two round-1/round-2 regression pins and the #747
composition test above). Every number below was observed by editing the hunk
out, running, and restoring:

revert result
the tick keeps discarding the report (src/core/daemon/runtime.js) 1 fail - the daemon maintenance tick persists what it left fragmented into status.json
no render, no --json key (src/core/commands/status.js) 2 fail - the hyp status names the frozen partitions... and an install with nothing frozen... tests
delete the diagnostic block in collectHypAwareStatus 1 fail - hyp status names the frozen partitions...
delete both retention caps (write side and read side) 2 fail - the named list is capped in the walk order... and a foreign status file is capped, cleaned...
delete the write-side sanitizeLabel 1 fail - summarizeMaintenanceSkips sanitizes and clamps hostile dataset and partition labels on write...
delete the if (p.compacted || p.rebaselined) return undefined guard 1 fail - a converged, rebaselined, or freshly rewritten partition is not on the surface

Two of them are not pure-unit: one seeds a partition frozen exactly the way
#723's was, runs the real maintainCache, and summarizes what comes back (so
the vocabulary is pinned to what a real walk produces); one boots the real
runDaemon with interval_minutes: 0.01 over that cache and polls
status.json for the snapshot.

Checks: CI green on all 9 checks at this head. At the last fully re-measured
head (69a0516, fresh npm install): npm test 4037 pass / 0 fail / 1
skipped; npm run typecheck clean; smokes cache_lifecycle_maintenance,
status_diagnostics, daemon_foreground_start_stop all ok.

What I could not verify

  • Real installed-daemon behaviour. The daemon test runs runDaemon in
    foreground with a 600 ms maintenance interval; nothing here exercises
    launchd/systemd, a real hourly cadence, or a long-lived process.
  • macOS. Everything was run on Linux.
  • A production cache with more than 8 frozen partitions. The cap, the exact
    total, and the ... and N more line are pinned by unit test only.
  • walkthrough_picker_to_first_query was not run: it fails on master for an
    unrelated reason (walkthrough_picker_to_first_query is red on master, and no smoke runs in CI #750).
  • The --json maintenance key is new, so no existing consumer pins it; I
    did not survey downstream consumers of hyp status --json outside this
    repo.

Known, deferred to #764

Non-blocking items triage left open rather than fixing here: the privacy
sentence at llp/0228:102-104, the foreign-file reason floor, one over-claiming
test comment, failedAt unsanitized on the write side (our own code only ever
writes an ISO timestamp there, and the read side clamps it), and, until commit
ca4510c, one leftover of the renumbering: the Extended-by: entry this PR adds
to llp/0218 read LLP 0226 and linked a filename that does not exist, while
LLP 0226 on master is the prune decision, so the forward-ref resolved to the
wrong document. That one is now fixed on this branch and points at LLP 0228;
the rest remain tracked on #764.

A partition maintenance deliberately leaves fragmented (LLP 0217, LLP 0218)
was stated only on surfaces an operator has to go looking for: a
`hyp query maintain` line and a `maintenance.partition` span attribute. The
daemon's hourly tick awaited `maintainCache` and discarded the report.

The tick now summarizes that report into `DaemonStatus.maintenance` and
persists it with the snapshot it already writes; `hyp status` renders the
block, carries it under `--json`, and raises one warning diagnostic, and the
tick logs one line when the count is nonzero. Retention is the last completed
tick, whole: both reasons are re-derived from the partition cursor every tick,
so the newest tick is the only current answer and a thawed partition leaves
the surface by itself. Bounded by a fixed reason-key set and an 8-entry named
list taken in the walk's own neediest-first order, with the exact total beside
it. The reason ids are the span attribute names verbatim.

Design record: LLP 0224, with Extended-by forward-refs on LLP 0217 and 0218.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral review - round 1

Head reviewed: 1129a1276709c7007a44c55ef16a38f1fc7e52be. All 9 checks SUCCESS.
Reviewed in a detached worktree; nothing was written to the branch.

The reviewer went at the bound dimension by dimension rather than reading the cap
and accepting it, and found the gap: the entry count is bounded on both sides,
but the string lengths inside those entries are bounded only on read. LLP 0224
says both. A realistic row-derived partition label reached 217 characters, and a
synthetic one 51,489. Worse, the daemon log line has no read side to save it, so
tail -f on the daemon log can be fed a line that reorders or hides itself.

Finding 3 is worth reading even though it is not this PR's to fix: after #747
lands, a partition whose rewrite throws on every tick reaches this surface not at
all. The reviewer confirmed the author's claim that failed correctly stays off
(an operator sees nothing rather than a false "0 skipped"), then traced why that
partition also never acquires either named reason: on the ordinary growth path a
throwing rewrite writes no LLP 0217 stamp, so it re-attempts and re-throws forever
and compactionAttemptFailed is never set. That is the one frozen-partition state
that still requires reading the log, which is what LLP 0224 exists to end.


Cleanup done, tree clean. Here is the review.


VERDICT: findings

PR #759 is well-built: the surface it adds is genuinely bounded in the dimension it set out to bound (entry count), the retention rule works as claimed, all 11 new tests discriminate, and npm test (4035/0/1 skipped), npm run typecheck, and the three named smokes are green on a fresh install. Three findings, none a blocker. The write-side gap is the one worth fixing before merge because a design doc claims the property it lacks.


1. major — src/core/daemon/status.js:531-532, src/core/daemon/runtime.js:711-713, llp/0224-...decision.md:95-98

The sanitizing is applied on read only. LLP 0224 and the PR body both say it is applied on write as well.

LLP 0224 line 95-96 states: "The cap and the sanitizing are re-applied on read as well as on write." On write, only the cap exists (if (partitions.length >= MAX_SKIPPED_PARTITIONS_REPORTED) continue). summarizeMaintenanceSkips pushes dataset: p.dataset and partition: partitionLabel(p.partition) raw — no sanitizeLabel, which is already imported in this file at line 22 and is the helper the sibling recentEntrypointsFromSources uses for exactly this reason.

This is not theoretical. Partition values are row-derived: resolveSourceSegments takes client_name off a captured row and passes it through sanitizePathSegment, which strips only [\x00-\x1f/\\:*?"<>|]. It does not strip DEL, C1 controls, soft hyphen, zero-width, or bidi overrides, and it applies no length clamp. I confirmed end-to-end through a real cache directory and a real maintainCache walk that a client_name of claude<U+202E><U+200B><U+00AD><DEL> + 200 chars produces:

written partition label length: 217
bidi in written label      : true
zero-width in written label: true
DEL in written label       : true

Two consequences:

  • status.json carries the raw bytes at full length. maintenanceSkipsFromStatus clamps to MAX_LABEL_CHARS (120) and strips on read, so the hyp status terminal render is safe — that part of the defense genuinely works.
  • runtime.js:711 has no read side. fileLog.warn('daemon.maintenance_skipped', { worst: \${skips.partitions[0].dataset}/${skips.partitions[0].partition}` })writes the write-side value into the daemon log.openDaemonLogemits viaJSON.stringify (src/core/daemon/logs.js:55), which escapes < 0x20but passes U+202E, U+200B, U+00AD and U+007F through verbatim. An operator runningtail -fon the daemon log gets a line that can reorder or hide itself. That is precisely the classsanitizeLabel exists for, and the repo is actively formalizing it (llp/0225-captured-text-is-escaped-for-display.decision.md, on fix/issue-752`).

Why it matters: a design doc asserting a safety property the code does not have is worse than not asserting it, because the next reader stops checking. And the log line is the one surface on this path with nothing downstream to clean it.

Exact fix. In summarizeMaintenanceSkips, replace lines 531-532 with:

      dataset: sanitizeLabel(p.dataset) ?? 'unknown',
      partition: sanitizeLabel(partitionLabel(p.partition)) ?? 'all',

That makes the LLP's sentence true, bounds status.json, and fixes the log line for free since it reads skips.partitions[0]. No test changes needed; all 11 still pass. Alternatively, if the author prefers to leave the write side raw, change LLP 0224:95-96 to "The cap is applied on write; the cap and the sanitizing are applied again on read" and sanitize the worst field at runtime.js:711 — but the one-line fix above is strictly better.


2. minor — src/core/daemon/status.js:456, :616, :986; src/core/commands/status.js:470-473

A status.json carrying a reason id this build does not know renders an empty parenthetical and an impossible count.

describeMaintenanceSkipReasons filters to reasons with a count > 0 and joins. When no known reason has a nonzero count but skippedTotal > 0, it returns '', and both call sites interpolate it unconditionally. Separately, partitionsVisited falls back to partitions.length (:616) with no floor at skippedTotal, while skippedTotal is floored at partitions.length (:619).

Observed, feeding a snapshot written by a hypothetical later build that added a third reason id — the exact forward-extension path LLP 0224#consequences endorses ("a new id and a new count key, not a new shape"):

  maintenance:
    3 of 3 partitions left fragmented, as of the tick just now ()
    ... and 3 more (hyp query maintain --dry-run lists them all)
  diagnostics:
    [WARN ] maintenance_partitions_skipped: cache maintenance is leaving 3 partitions fragmented (), as of its tick at 2026-08-14T00:12:40.848Z

And with partitionsVisited absent ({ tickAt, skippedTotal: 5, reasons: {}, partitions: [] }):

    5 of 0 partitions left fragmented, as of the tick just now ()
    ... and 5 more (hyp query maintain --dry-run lists them all)

5 of 0 is not a sentence any tick can produce. Neither is reachable from a file this build wrote, but the read path's entire stated justification is that this build did not necessarily write it, and the same 60 lines already handle the harder cases (ESC bytes, unknown ids, negative counts) correctly. Note the existing test at line 186 exercises an unknown reason id but keeps compaction_ineffective: 900 alongside it, so the all-unknown case is never hit.

Exact fix. Three edits:

  1. status.js:616 — floor visited at the total:
    partitionsVisited: Math.max(nonNegativeInt(raw.partitionsVisited) ?? 0, recordedTotal, partitions.length),
    (move the recordedTotal const above the return).
  2. status.js:456, in describeMaintenanceSkipReasons, after the join: return a fallback when empty, e.g.
    const phrase = MAINTENANCE_SKIP_REASONS.filter(...).map(...).join(', ')
    return phrase === '' ? 'reasons this build does not recognize' : phrase
    which fixes both status.js:986 and commands/status.js:473 at once.
  3. Extend the a foreign status file is capped, cleaned... test with an all-unknown-reason case asserting the rendered line has no ().

3. minor — src/core/daemon/status.js:498-503 (skipReasonOf), interaction with PR #747

After #747 merges, a partition whose rewrite throws on every tick never reaches this surface at all.

The author's claim that "failed is a different fact and correctly stays off this surface" is right as far as it goes, and I verified it: against a #747-shaped report ({ failed: true, errorKind: 'decode' }), summarizeMaintenanceSkips returns skippedTotal: 0, the text block is suppressed (the gate is skippedTotal > 0), and no diagnostic is raised. So an operator does not see a false "0 skipped" — they see nothing, which is the correct behavior for a surface that only speaks about two named reasons. Good.

The residual gap: src/core/cache/maintenance.js:424 writes the LLP 0217 retry stamp only if (verdictStale). On the ordinary grewSinceCompaction path a throwing rewrite writes no stamp, so the next tick sets compactionDue again, attempts again, and throws again. compactionAttemptFailed is never set, so that partition sits fragmented forever and this surface never names it. Under #747 it produces a daemon.maintenance_failed line every hour — but the whole premise of LLP 0224#status-file-is-the-surface is that the operator should not have to read the log to find a frozen partition, and this is the one frozen-partition state that still requires it.

Not a defect in this PR (it is out of LLP 0224's stated scope, and the shape takes the fix additively as the doc says). Raising it so it is on the record when #747 lands rather than rediscovered later.

Suggested fix (follow-up, not this PR). Add a third id maintenance_failed to MaintenanceSkipReason and a third reasons key, set from p.failed in skipReasonOf, once #747 is in. No shape change; the render, the cap, and the read-side validator all absorb it. Alternatively, add a sentence to LLP 0224#reason-ids-are-span-attribute-names naming this as knowingly deferred, so the doc's "deliberately not reasons" list is complete.

On the conflict claim itself: verified and accurate, with one addition. #747 touches 14 files; the only two that collide with #759 are the two the author named — src/core/daemon/runtime.js (runMaintenance: #747 replaces withSpan with getTracer('daemon').startActiveSpan(...) plus a try block, for the reason LLP 0220#tick-reports-degraded gives about withSpan's status-attribute snapshot) and the **Extended-by:** line on llp/0218. The proposed resolutions are correct and mechanical. #747's other files (llp/0199, src/core/cache/{maintenance.js,types.d.ts}, src/core/commands/query.js, two smoke flows, five test files including a new test/core/daemon-maintenance-tick-status.test.js) are all disjoint from #759's. The author's file list omits the two smoke flows and the test files, but none of them touch anything #759 touches, so the conclusion stands. #747's span.setAttribute('partitions_maintained', ...) and #759's partitions_visited compose without collision. LLP numbers 0220 and 0224 are both free and distinct.


Bounds, tested

Every input I tried to grow without limit, and what happened. Probes ran against the real summarizeMaintenanceSkips / maintenanceSkipsFromStatus / collectHypAwareStatus / renderStatusText / renderStatusJson, plus one end-to-end run through a real cache directory and a real maintainCache walk.

Input Write side Read side Verdict
Number of named partitions (20 skipped, then 40 in a foreign file) capped at 8, walk order preserved (dataset_00..dataset_07) capped at 8 again, break before the loop body bounded, both sides
Partition tuple string k=v/k=v unbounded. 200 keys × 250-char values → a 51,489-char label. Realistic row-derived case → 217 chars clamped to 120 by sanitizeLabel finding 1
Dataset name unbounded. 5,000 chars written verbatim clamped to 120 finding 1
Display-hostile bytes in the label (ESC, LF, DEL, U+00AD, U+200B, U+202E) all survive into status.json and into the daemon log's worst field; sanitizePathSegment strips only \x00-\x1f and path chars, so bidi/zero-width/DEL reach the directory name from a row's client_name ESC/LF/bidi/zero-width all stripped before render finding 1; hyp status itself is safe
Number of distinct reason keys fixed 2-key literal, nothing derived from the report fixed 2-key literal; unknown keys never enter reasons (verified: compaction_from_the_future: 5 dropped) bounded
Reason payload (dataFiles, failedAt) typed guards (typeof === 'number' / 'string') nonNegativeInt / sanitizeLabel (120) bounded
status.json total size across many ticks persist is Object.assign + full rewrite; every tick overwrites maintenance whole. 8 pathological entries → 102,063 bytes; the very next clean tick → 291 bytes n/a no accumulation; a clean tick does shrink it back. LLP 0224's "bounded constant, no growth over a daemon's lifetime" holds in the count dimension, not the string-length dimension
skippedTotal vs the named list exact count of partitions matching a named reason Math.max(recordedTotal, partitions.length) — never smaller than the list it labels (verified: recorded 1, list 3 → 3) honest
partitionsVisited report.partitions.length; the budget break happens before reports.push, so only genuinely-visited partitions are counted falls back to partitions.length with no floor at skippedTotal5 of 0 partitions finding 2
... and N more arithmetic n=0 → no block, no diagnostic. n=7 → 7 named, no more-line. n=8 → 8 named, no more-line. n=9 → 8 named, "and 1 more". n=20 → 8 named, "and 12 more" same correct at every boundary
Empty / non-string partition tuple {}all; undefinedall; {a: 1, b: 'ok'}b=ok (non-string value silently dropped — unreachable, CachePartitionMeta.partition is Record<string,string> from path parsing) n/a safe; the last case is a nit, not a finding
Reason ids not in the vocabulary (baseline-gate convergence, rebaseline, a just-rewritten ineffective partition, #747's failed) skipReasonOf returns undefined; not counted in skippedTotal, not in reasons, not in the list n/a skippedTotal counts partitions skipped for a named reason, exactly as MaintenanceSkipSnapshot.skippedTotal's JSDoc says. Nothing is miscounted as one of the two named ids; see finding 3 for what stays off
Old status.json with no maintenance key n/a isPlainObject(undefined)null. No throw, --json emits null, text renders nothing, overall: healthy forward/backward compatible
Missing or unparseable tickAt n/a returns null outright safe
Future tickAt n/a formatEntrypointAgejust now (negative delta < 60) acceptable; pre-existing helper

Also checked, clean

Ran (fresh npm install, exit 0):

Discrimination, re-derived (3 of the author's 5, all matched exactly):

  • git checkout c483c1a -- src/core/daemon/runtime.js1 fail / 10 pass, the daemon maintenance tick persists what it left fragmented into status.json. ✓
  • Delete both cap lines (write continue + read break) → 2 fail / 9 pass, the named list is capped... and a foreign status file is capped.... ✓
  • Delete if (p.compacted || p.rebaselined) return undefined1 fail / 10 pass, a converged, rebaselined, or freshly rewritten partition is not on the surface. ✓

The daemon test as a CI citizen (test/core/status-maintenance-skips.test.js:449): bounded 30 s deadline with 100 ms polling; assert.ok(maintenance, 'no maintenance snapshot ever reached status.json') gives a real failure message; not vacuous (asserts skippedTotal === 1, the exact reasons object, dataset and reason); finally does handle.stop(), await handle.done, and fs.rm(hypHome, { recursive: true, force: true }); installSignalHandlers: false. The 600 ms interval_minutes: 0.01 against a 30 s budget is ~50 ticks of headroom, and the observed run needed one — stable on a slow runner. The assert.match(diagnostic.message, /3 partitions fragmented|leaving 3 partitions fragmented/) at line 304 is a weak assertion (the second alternative subsumes the first) but not vacuous. No flake risk found. Not covered: the ... and N more render at exactly 8 vs 9 (I verified both by hand, both correct).

hyp status contract: text block gated on skippedTotal > 0, --json always carries the key (null before any tick) — consistent with LLP 0224#consequences bullet 1 and with the surrounding renderer's conventions. A never-ticked daemon renders nothing in text and null in JSON — no undefined, no 0 partitions skipped. --json uses snake_case (tick_at, partitions_visited, skipped_total, data_files, failed_at) matching the rest of renderStatusJson; the key is skipped where the internal field is partitions, which is fine since a JSON consumer reads capping off skipped_total vs skipped.length. The maintenance_partitions_skipped diagnostic is severity: 'warning' and the kind is not in degradingKinds (new Set(['config_missing', 'config_unreadable']) at status.js:1345), so it cannot reach the overall computation's degrade branches — confirmed by observation (overall: healthy in every probe that raised it). StatusDiagnosticKind is extended correctly in the union. No in-repo consumer pins the --json key set exhaustively; the status_diagnostics and cli_bundled_plugins_activated smokes only assert exit 0 and parseability.

LLP 0224: number is free — I enumerated llp/02[19-25] across master and all 30 remote branches; 0219 (fix/issue-736), 0220 (fix/issue-737/#747), 0221 (fix/issue-744), 0222 (fix/one-pushdown-converter), 0223 (fix/issue-746), 0225 (fix/issue-752) are claimed, 0224 only by this branch. Every @ref added by the PR resolves: 0164#not-liveness-gated, 0218#verdict-outranks-error, 0224#{status-file-is-the-surface,last-tick-only,reason-ids-are-span-attribute-names}, plus 0199#{neediest-first,baseline-gate}, 0217#{record-effectiveness,retry-on-writer-change}, 0218#report-the-spent-attempt referenced in prose. Claims verified against code: the reason ids are the span attribute names verbatim (maintenance.js:366, :379); walk order is neediest-first with no re-sort (maintenance.js:135-138, and summarizeMaintenanceSkips adds none); the summarizer stats nothing; partitionsVisited genuinely excludes budget-cut partitions. The Extended-by: additions are additive — 0217 appends ; [LLP 0224]... to an existing line, 0218 adds a new line after **Related:**; nothing either doc settled is rewritten. LLP 0224:95-96 is the one claim that does not hold (finding 1).

Conventions: no em dash (U+2014) anywhere in the diff — grepped the full patch, zero hits. No statement-terminating semicolons in the new JavaScript (the four ; hits are sentence punctuation inside pre-existing prose comments). No @typedef in any touched file — the two new types in src/core/daemon/types.d.ts are a type union (MaintenanceSkipReason) and two interfaces (MaintenanceSkippedPartition, MaintenanceSkipSnapshot), imported via @import. No inline import('...') types. Type-import specifiers are root-anchored .js paths throughout ('../../../src/core/daemon/types.js', the new '../../../src/core/cache/types.js' in status.js, '../../src/core/cache/types.js' and '../../hypaware-plugin-kernel-types.js' in the test).

On weight. +1035 lines is a lot for a status field, but the split is defensible: 495 test, 162 LLP, and of the ~380 production lines a large majority is JSDoc prose. The functional core is roughly 90 lines. The one place I considered calling gold-plating — the ~60-line maintenanceSkipsFromStatus re-validating a file only the daemon writes — is the repo's established convention (recentEntrypointsFromSources does the same for the same stated reason), and finding 1 shows the read side is currently the only thing standing between a row-derived label and a terminal, so it is earning its keep. I would not cut anything. If asked to trim, the honest target is the JSDoc-to-code ratio in types.d.ts, not any of the logic.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review complete. Findings below; the tree is clean and all scratch removed.


neutral review - round 2 (final)

Head reviewed: 69a05160977ac89022878459abbae21662b3fc3b. All 9 checks SUCCESS, and I re-ran the gates on a fresh install both at this head and at this head merged with the moved master (8c08185f). Reviewed in a detached worktree; nothing was written to the branch.

The round-1 major is genuinely fixed and the fix is load-bearing under mutation. The two new tests both discriminate, and the on-disk-bytes assertion is not vacuous. What I found this round is one false sentence left standing in LLP 0224 (a different sentence from the one round 1 flagged, and one this PR's own new code comment contradicts), one case where the new fallback phrase makes a positively wrong claim instead of an obviously blank one, and three nits.

VERDICT: findings

None of them is ship-blocking. The feature works, the round-1 major is closed, and everything round 1 cleared still holds. Findings 1 and 2 are worth a follow-up commit if triage is willing to take one; findings 3-5 are nits and I would not hold the PR for any of them.


1. minor - llp/0224-maintenance-skips-are-a-standing-surface.decision.md:102-104

LLP 0224 still says nothing on this path comes from a row. This PR's own code comment now says the opposite.

Round 1's finding 1 was that 0224:95-96 claimed a property the code lacked. That is fixed. But nine lines later the same section still says:

Those are dataset and partition identifiers and kernel-side counters. Nothing from a row, a prompt, a credential, or a config value is anywhere near this path.

That is false, and the fix commit's own comment at src/core/daemon/status.js:546-548 says so in as many words: "partition's values come off a captured row's client_name by way of resolveSourceSegments -> sanitizePathSegment". Re-derived independently: discoverCachePartitions (src/core/cache/partition.js:190-194) builds the tuple by splitting each directory name at the first =, and those directory names are written by resolveSourceSegments (partition.js:301-311) as `source=${sanitizePathSegment(source)}` where source is row[col] for a declared source column. A row value is not merely near this path, it is the partition tuple's value.

Why it matters: this is exactly the failure mode round 1 named. A future reader deciding whether this surface needs escaping reads "nothing from a row is anywhere near this path" and stops checking. It is more dangerous now than before the fix, because the sanitizing that makes the code safe reads as belt-and-braces rather than as the thing standing between a client-chosen string and an operator's terminal, so a later refactor can remove it in good conscience. The paragraph is also the doc's only privacy statement about the surface, and it is wrong about which fields are free-form.

Exact fix. Replace the last sentence of that paragraph with something true, e.g.:

partition identifiers and kernel-side counters. The partition tuple's values
are the exception: they are row-derived (a captured row's `client_name`
through `resolveSourceSegments` -> `sanitizePathSegment`, which strips only
path-hostile bytes), which is why the write side sanitizes and clamps them
rather than trusting them. No prompt, credential, or config value is on this
path.

Editing this is legitimate: 0224 has not merged, so it is not yet a settled record, and the fixer already amended the same section in this commit.


2. minor - src/core/daemon/status.js:471, and :610-646

The new fallback phrase can appear while the reasons are recognized, contradicting the lines printed directly beneath it.

The task for this round was to check that 'reasons this build does not recognize' cannot appear when reasons are recognized. It can. describeMaintenanceSkipReasons reads only the reasons counts; maintenanceSkipsFromStatus floors skippedTotal at partitions.length (:646) but never floors the per-reason counts at the named list. So a status.json whose reasons object is absent, empty, or zeroed while its partitions array carries recognized reason ids produces:

  maintenance:
    1 of 1 partitions left fragmented, as of the tick just now (reasons this build does not recognize)
    - ai_gateway_messages/source=claude  [compaction_ineffective]  the last rewrite of 12 files reduced nothing
  diagnostics:
    [WARN ] maintenance_partitions_skipped: cache maintenance is leaving 1 partition fragmented (reasons this build does not recognize), as of its tick at 2026-08-14T00:41:38.553Z

The build recognizes the reason well enough to print it, gloss it, and pick its detail phrasing one line down. Observed from { tickAt, skippedTotal: 0, reasons: {}, partitions: [{ ..., reason: 'compaction_ineffective', dataFiles: 12 }] } through the real collectHypAwareStatus + renderStatusText.

The same un-floored breakdown produces a milder version with a partial reasons object, and there is already a test pinning it: test/core/status-maintenance-skips.test.js:278 asserts reasons is { compaction_ineffective: 900, compaction_attempt_failed: 0 } for a fixture whose named list is ['compaction_ineffective', ...7 x 'compaction_attempt_failed']. That renders (900 compaction_ineffective) above seven [compaction_attempt_failed] lines the breakdown never mentions.

Why it matters: it is the same class as round-1 finding 2, whose whole justification the fixer accepted ("a file this build did not write can claim whatever it wants"). Pre-fix this case rendered a bare (), which reads as obviously broken; post-fix it renders a confident, specific, and wrong sentence. That is a small step backwards on this sub-case even though it is a step forwards on the all-unknown case.

Exact fix. Floor the per-reason counts at the named list, mirroring the skippedTotal floor two lines below. In maintenanceSkipsFromStatus, before the return:

  // The named list is the ground truth for reasons it does name: a count
  // smaller than the entries carrying it would print a breakdown the list
  // right below it contradicts.
  for (const p of partitions) {
    if (reasons[p.reason] < 1) reasons[p.reason] = 1
  }

or, exactly parallel to skippedTotal, tally the list and Math.max each key. Then add the recognized-reason case to the new test at :437 (assert the rendered breakdown names compaction_ineffective, not the fallback), and update :278's expectation to compaction_attempt_failed: 7.


3. nit - test/core/status-maintenance-skips.test.js:279-283

Finding 2's fix retired the "a negative count is not a count" guard, and the comment still claims it.

The new floor is Math.max(nonNegativeInt(raw.partitionsVisited) ?? 0, recordedTotal, partitions.length). Both recordedTotal and partitions.length are >= 0 by construction, so a negative first argument can never be observable at this call site. Verified by mutation: replacing nonNegativeInt(raw.partitionsVisited) ?? 0 with typeof raw.partitionsVisited === 'number' ? raw.partitionsVisited : 0 (i.e. letting -1 straight through) leaves the file at 13 pass / 0 fail. The assertion's own comment still opens "A negative count is not a count", but nothing in the file can fail if that stops being true.

This is not a behavior defect: the floor makes the outcome safe either way, and partitionsVisited: 900 is correct rather than convenient (see the re-derivation below). It is a comment that over-claims what its assertion proves.

Exact fix. Drop the first clause of the comment at :279, keeping the part that is now tested:

  // 900 partitions were recorded skipped, so visited is floored at the
  // skipped total rather than at the capped list length: "8 of 900" is
  // exactly the impossible-looking sentence a missing floor would render.
  // (The negative `partitionsVisited` in this fixture is unobservable now:
  // the floor is at least `recordedTotal`, so `nonNegativeInt` here is
  // defense in depth rather than the thing under test.)

4. nit - src/core/daemon/status.js:562-564, llp/0224:95-96

failedAt is still written to status.json unsanitized, so 0224:95-96 is true of two of the three free-form strings, not all three.

The fix sanitizes dataset and partition. failedAt is still p.compactionAttemptFailedAt verbatim, guarded only by typeof === 'string'. Its source, compactionAttemptFailedAt (src/core/cache/maintenance.js:1218-1224), reads cursor.compaction.attemptFailedAt off the on-disk cursor JSON and checks only that it is a non-empty string - no strip, no clamp. That is the same trust boundary (the cache directory) the fixer just decided partition labels sit on.

Materially smaller than round-1's finding: our own code only ever writes an ISO timestamp there, failedAt never reaches the daemon log's worst field, and the read side clamps it before any terminal. The exposure is status.json bytes only, and 8 entries x an unbounded string is the one way left to breach 0224#consequences' "grows by a bounded constant". Worth a token for completeness rather than for risk.

Exact fix. At :562-564:

      ...(reason === 'compaction_attempt_failed'
        ? { ...(sanitizeLabel(p.compactionAttemptFailedAt) !== undefined
          ? { failedAt: /** @type {string} */ (sanitizeLabel(p.compactionAttemptFailedAt)) }
          : {}) }
        : {}),

or hoist a const failedAt = sanitizeLabel(p.compactionAttemptFailedAt) above the push.


5. nit - llp/0224:125-133 (the new deferred-scope paragraph)

The new sentence is accurate about the partition and silent about the tick.

Its claims check out: maintenance.js:424 writes the LLP 0217 stamp only if (verdictStale), so on the grewSinceCompaction path a throwing rewrite never sets compactionAttemptFailed, and this surface never names that partition. Confirmed.

What it omits is that, as of this PR's base, a throwing rewrite does not merely go unnamed - it ends the whole tick. maintainCache's per-partition loop (maintenance.js:147-200) has no catch, withSpan rethrows (src/core/observability/span_helpers.js:45), and the throw err at maintenance.js:434 therefore propagates out of maintainCache into runtime.js's .catch, so summarizeMaintenanceSkips(report) and persist({ maintenance: skips }) at runtime.js:697-699 never run. Combined with neediest-first ordering, which puts the likeliest-to-throw partition first, the walk aborts at iteration 0 and the entire snapshot freezes at the last completed tick - including for every other partition, and including the "a clean tick shrinks it back" property. If no tick has ever completed, the field is simply absent and the render says nothing.

Mitigating, and why this is a nit rather than a finding: PR #747 ("One partition's failure no longer ends the maintenance walk") is open and fixes exactly this, after which the paragraph is accurate as written. The author already flagged #747 as the interacting PR.

Exact fix (optional). Append one clause: "...and this surface says nothing about it. Until #747 lands the tick itself throws, so the snapshot is not rewritten at all that tick and holds the last completed tick's answer, with its age."


Round-1 findings, re-derived

1. major, sanitizing on write - FIXED, and the fix is load-bearing. status.js:554-555 now applies sanitizeLabel(...) ?? 'unknown' / ?? 'all'.

  • The daemon log's worst field, verified independently of the fixer's script: I built `${s.partitions[0].dataset}/${s.partitions[0].partition}` exactly as runtime.js:711-713 does, from a report whose dataset and partition.source were both claude + U+202E + U+200B + U+00AD + DEL + ESC + LF + 300 filler chars. Result: 241 chars, and includes() false for every one of bidi, zero-width, soft hyphen, DEL, ESC and LF. Round 1's tail -f hole is closed.
  • Not vacuous, and it tests both halves. Mutation to strip-only (sanitizeLabel(x, 1e9)): 1 fail / 12 pass. Mutation to clamp-only (String(x).slice(0,120)): 1 fail / 12 pass. Full revert: 1 fail / 12 pass, matching the fixer's report.
  • The on-disk-bytes assertion is real. Making writeStatusFile a no-op for a healthy status turns the file red in 5 tests including this one (readFile throws on the missing path). It cannot pass with no file. And the four characters it asserts against (U+202E, U+200B, U+00AD, U+007F) are all characters JSON.stringify does not escape, so the assertion is testing the sanitize rather than the serializer.
  • The ?? 'unknown' / ?? 'all' fallbacks do not mislead. ?? 'all' is unreachable dead code: partitionLabel returns either the literal 'all' or a k=v join, and = and / both survive UNSAFE_LABEL_CHARS, so the result is never empty (probed {}, undefined, {ZW: ZW} -> "=", {BIDI: BIDI} -> "=", {a: ''} -> "a="). A legitimate partition therefore cannot be confused with the literal all that means "no partition keys". ?? 'unknown' is reachable - a dataset name of only invisible characters yields 'unknown' - and that is fine: unknown is not a registered dataset name, so it reads as the placeholder it is.
  • Clamping can make two distinct partitions render identically (source= + 130 p + AAA and the same + BBB both clamp to the same 120-char string ending ...). I judge this does not matter: they remain two separate lines, the ... announces truncation, skippedTotal is unaffected, and the render already points at hyp query maintain --dry-run for the authoritative enumeration. The one place it costs something is the log's worst field, which becomes ambiguous between two such partitions - acceptable, since worst is a pointer and the status file is the discovery surface.
  • Ordering is untouched. The premise that "the write-side sanitize now runs before the cap" is not what the code does: the cap check if (partitions.length >= MAX) continue is at :551, above the push whose properties carry the sanitize. Sanitizing runs only on entries that already survived the cap, so it cannot change which 8 are named or their order. Confirmed empirically at n=20: named = ds_00..ds_07, i.e. walk order, neediest first.

2. minor, empty parenthetical + impossible count - FIXED, but with a new problem. The two cases round 1 named are gone: { skippedTotal: 5, reasons: { compaction_from_the_future: 5 }, partitions: [] } with no partitionsVisited now renders 5 of 5 partitions left fragmented ... (reasons this build does not recognize) in both the text block and the diagnostic, no () and no 5 of 0. The new problem is finding 2 above: the fallback now fires in a case where the reasons are recognized.

  • Is partitionsVisited: 900 correct rather than convenient? Yes. On the write side partitionsVisited = visited.length and skippedTotal counts a strict subset of visited (every increment is inside the same loop, past a skipReasonOf guard), so skipped <= visited holds identically for every snapshot this build writes - the floor is a provable no-op on our own files and can only ever fire on a foreign one. There is no real snapshot where visited legitimately reads lower than skipped; a budget-cut tick (max_tick_ms) still only counts partitions it actually pushed a report for, so the cut lowers both together. 900 of 900 is the honest reading of a file that claims 900 skips: the file is silent about visited, and the smallest number consistent with its own claim is 900. The old expectation of 8 was the capped list length standing in for a walk count, which is the coincidence, not the floor.
  • The changed expectation does weaken one guard as a side effect - see finding 3.
  • Mutation: reverting the floor is 2 fail / 11 pass (a foreign status file is capped... + the new fallback test), matching the fixer's report. Reverting the describeMaintenanceSkipReasons fallback alone is 1 fail / 12 pass.

3. deferred, always-throwing rewrite - ADDRESSED as instructed, doc only. One paragraph added at llp/0224:125-133, no code. Its factual claims verify (see finding 5); it understates the blast radius as of this base, which is a nit given #747.

Also checked, clean

Ran (fresh npm install, exit 0, at head 69a0516):

  • npm test - 4038 tests, 4037 pass / 0 fail / 1 skipped (round 1 saw 4036/4035; +2 for the new tests).
  • npm run typecheck - clean.
  • node --test test/core/status-maintenance-skips.test.js - 13/13.
  • Nine mutations of src/core/daemon/status.js, each reverted immediately: write-sanitize revert (1 fail), fallback revert (1), floor revert (2), negative passthrough (0 - finding 3), write cap removed (1, the named list is capped...), read cap removed (1, a foreign status file is capped...), writeStatusFile no-op (5), clamp-only (1), strip-only (1).

Base interaction with the moved master (8c08185f, PR #721) - clean. File sets are fully disjoint: #721 touches llp/0222, package.json, src/core/query/parquet-pushdown.js, test/core/parquet-source.test.js; #759 touches none of them. git merge --no-commit origin/master reported "Automatic merge went well" with zero conflicts. On the merged tree with a fresh npm install (which picks up the squirreling 0.15.3 / icebird 0.8.22 / hyparquet 1.28.2 bumps): npm test 4043 tests, 4042 pass / 0 fail / 1 skipped, npm run typecheck clean, and npm run smoke -- status_diagnostics / cache_lifecycle_maintenance / daemon_foreground_start_stop all ok. The dependency bump was the only plausible coupling (this PR's daemon test exercises the real Iceberg write path through appendRowsToSourceTable and maintainCache) and it is unaffected. The merge was aborted and the worktree reset; package_bin_boot and walkthrough_picker_to_first_query not run, red on master per #758/#750.

Regressions in what round 1 cleared, re-derived:

  • Cap on both sides. Write: 20 skips -> partitions.length === 8, skippedTotal === 20. Read: a foreign file with 40 entries -> 8 kept, d00..d07, break before the loop body. Both mutations discriminate.
  • Clean-tick shrink. 8 pathological entries (400-char dataset, 4000-char partition each) written through the real writeStatusFile -> 3,279 bytes on disk; the very next clean tick -> 291 bytes. The write-side clamp is why the pathological number is now 3,279 rather than round 1's 102,063, which is the fix doing its job.
  • ... and N more boundary arithmetic, through the real renderStatusText: n=0 no block at all; n=7 -> 7 named, no more-line; n=8 -> 8 named, no more-line; n=9 -> 8 named, "and 1 more"; n=20 -> 8 named, "and 12 more". Correct at every boundary, unchanged by the fix.
  • Old status.json with no maintenance key: report.maintenance === null, --json emits "maintenance": null, text renders nothing, overall: healthy. No throw.
  • The warning cannot degrade overall: raised in every probe that had a positive skippedTotal, overall: healthy in all of them. A clean tick (skippedTotal: 0) raises no diagnostic and renders no block, while --json still carries the full zeroed snapshot.
  • skippedTotal honesty and the reason vocabulary: unchanged by this commit.

LLP 0224. The number is still free: across origin/master and every remote branch, llp/0224* exists only on origin/fix/issue-742. 0225 is origin/fix/issue-752 (PR #760, open), 0223 is fix/issue-746, 0226 unclaimed. The Extended-by: additions on 0217 and 0218 are additive forward-refs, appended rather than rewriting anything either doc settled. Every anchor the new @refs target (0224#last-tick-only, 0224#status-file-is-the-surface) exists. The rest of the doc still matches the code after these edits: the cap is 8 on both sides, the reason ids are still the span attribute names verbatim, walk order is still LLP 0199#neediest-first with no re-sort added, partitionsVisited still excludes budget-cut partitions, --json still carries null before any tick, and the #consequences extension story ("a new id and a new count key, not a new shape") is exactly what the added paragraph leans on. The two sentences that do not match are findings 1 and 4.

The @ref at status.js:553 points at 0224#last-tick-only, the same anchor already carried by the enclosing summarizeMaintenanceSkips. It is not mechanical duplication - the function-level ref is about the cap and the walk order, this one is about the write side sanitizing - but a reader may find the repetition odd. Not a finding.

Conventions: zero U+2014 in the full diff against the merge base. No statement-terminating semicolons in any added JavaScript line. No @typedef in any touched file; the two new types in src/core/daemon/types.d.ts remain a type union and two interfaces. No inline import('...') types. Type-import specifiers are root-anchored .js paths throughout, including the new test's '../../src/core/cache/types.js' and '../../hypaware-plugin-kernel-types.js'.

Weight of the round-2 commit: +163 / -5 across three files, of which 122 lines are test and 10 are LLP. The production delta is ~30 lines, most of it the two JSDoc/comment blocks explaining why the write side sanitizes and why the floor exists. Both explanations earn their place - each names a failure mode the code alone does not show. Nothing to trim.

Cleanup: all probe scripts removed; git status --porcelain in the worktree is empty at 69a0516. Nothing outside wt-759-r2 was touched.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral triage - ship

Review budget exhausted with five residual findings. All five are preferences.
This PR can merge.
Deferred to #764.

Baseline first, because it is what the residuals are weighed against: before this PR
a skipped partition's reason existed only as a span attribute, visible only to
someone who had tracing on at the moment of the tick. Nothing in the residual set
regresses below that.

The finding that needed a real decision

Round 2 found LLP 0224 still saying "Nothing from a row, a prompt, a credential, or
a config value is anywhere near this path"
, which is false, and which this PR's
own code comment contradicts
. Triage verified the row-derived claim itself rather
than inheriting it: resolveSourceSegments (src/core/cache/partition.js:301-311)
takes row[col], e.g. client_name, off a captured row and writes it into the cache
directory name via sanitizePathSegment, which strips only [\x00-\x1f/\\:*?"<>|]
with no clamp and no bidi, zero-width, DEL or C1 filtering; discoverCachePartitions
(:190-194) rebuilds the partition tuple by splitting those names at the first =.

The argument for blocking was that a false doc invites a future refactor to delete
the write-side sanitize as apparent belt-and-braces. What decided it the other way is
that the load-bearing knowledge is duplicated at the exact refactor site: the
12-line comment directly above the sanitize calls at status.js:542-553 names the
row-derived provenance, the sanitizePathSegment gap, and the log's worst field as
the unguarded surface. A refactorer removing the sanitize has to delete that comment
first, and the repo's keep-refs-honest rule sends them to the doc, where the
contradiction surfaces rather than hides.

So: a false sentence about correct, self-documenting code is a doc bug, just an
unusually important one. It is item 1 on #764, framed as a factual correction rather
than a design change.

The other four

The fallback phrase (reasons this build does not recognize) can fire on a foreign
status.json while the reasons are recognized, printing a confident wrong sentence
directly above the contradicting detail. Unreachable from any file this build writes
(summarizeMaintenanceSkips increments the count in the same loop that names the
partition), and unreachable from the forward-extension path LLP 0224 endorses; it
needs an internally inconsistent writer or a hand edit. Robustness polish on a
defensive read path.

The rest: a test comment claiming more than its assertion proves (the floor makes the
negative-count guard unobservable), failedAt still written unsanitized (our own code
only writes ISO timestamps there, it never reaches the daemon log, and the read side
clamps it), and the deferred-scope paragraph being silent that pre-#747 a throwing
rewrite aborts the whole walk rather than merely going unnamed. That last one is moot
if #747 lands first.

Verified at head 69a0516 after a fresh install: npm test 4037 pass / 0 fail / 1
skipped, npm run typecheck clean, node --test test/core/status-maintenance-skips.test.js
13/13, smokes status_diagnostics, cache_lifecycle_maintenance and
daemon_foreground_start_stop all ok. Round 2 additionally merged this head with the
moved master (8c08185f) and got a clean merge with 4042 passing.

@philcunliffe
philcunliffe marked this pull request as ready for review August 14, 2026 01:07
@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 14, 2026
philcunliffe added a commit that referenced this pull request Aug 14, 2026
…r a person (#752) (#760)

* Escape captured control sequences where a query result is rendered for a person (#752)

Co-Authored-By: Claude <noreply@anthropic.com>

* Renumber LLP 0224 to 0225: 0224 is claimed by PR #759 (#752)

* Fix round-1 review findings on escape-for-display (#752)

- LLP 0225 Verification: separate the six tests that discriminate the fix
  from the two that pass pre-fix by construction and guard preserved
  behaviour, confirmed by re-running the three new test files against the
  pre-fix sources and by mutation.
- format.js: move the receipt rationale and its @ref out of the `full`
  param's description and into the function description, so the @PARAM
  list is contiguous again.
- overview.js: clip before escaping for the model and tool columns,
  matching format.js's order, so a `\uXXXX` escape can no longer be cut in
  half at MAX_MODEL_WIDTH. cell() gains an optional width argument that
  truncates the raw value before escapeForDisplay.
- LLP 0225 Consequences: record the markdown newline behaviour change
  alongside the table one.
- Add a test pinning table column width to the escaped header rather than
  the raw column name (the one surviving mutant from review).

---------

Co-authored-by: test <test@example.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: test <test@test.com>
@philcunliffe philcunliffe removed the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 14, 2026
test and others added 2 commits August 14, 2026 19:05
Two textual conflicts, both anticipated by the PR description, plus two
semantic ones git could not flag.

src/core/daemon/runtime.js, runMaintenance: #747 (LLP 0220) replaced the
`withSpan` wrapper with a hand-rolled `tracer.startActiveSpan`, because the
tick's clean/degraded status is only knowable once the report is in hand.
Kept that skeleton, and put this branch's three statements back inside its
`try` after `const report`: `summarizeMaintenanceSkips`, `persist`, and the
`daemon.maintenance_skipped` line. The span attributes compose, with #747's
`partitions_failed` / `partitions_maintained` beside this branch's
`partitions_visited` / `partitions_skipped`, and #747's `setStatus` stays
last so it still reads the finished report.

llp/0218, the header: both sides appended an `**Extended-by:**` line. Merged
to one line carrying LLP 0220 then LLP 0228, separated by `; `.

LLP number collision: master landed 0224-desktop-setup-second-pass, so this
branch's 0224-maintenance-skips-are-a-standing-surface was a second claimant
on the same number and every `@ref LLP 0224#status-file-is-the-surface` would
have been ambiguous. Renumbered the later claimant to 0228 per LLP 0156, with
every inbound reference swept (llp/0217's forward-ref, the doc title, and the
refs in src/core/daemon/{runtime,status,types.d}.js, src/core/commands/
status.js, and the test). Refs carrying 0224's own anchors (#repair-surface,
#ask-once-per-pick) are the desktop doc's and were left alone. 0226 and 0227
are already claimed by open branches, so 0228 is the first free number above
the highest claimed anywhere.

MaintenanceReport.totalFailed: #747 made the field required, so this branch's
test fixture no longer typechecked. Derived it from the partitions like every
other total, and pinned the interaction it exposes with a new test: a
partition carrying #747's `failed` is this tick's error, not a skip, so it
stays off the skip surface and keeps its own per-partition
`daemon.maintenance_failed` line (LLP 0220#this-tick-versus-a-recorded-one).

npm test and npm run typecheck both produce a failure set byte-identical to
an origin/master worktree in the same environment (23 test failures, 1
typecheck error, all pre-existing and environmental). The PR's own suite is
14/14. Smokes cache_lifecycle_maintenance, status_diagnostics, and
daemon_foreground_start_stop are green.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral triage - re-triage after the master merges - ship

Head triaged: c51ed167bef2e0dc282648cd91d3d4ccd5c92d19. All 9 checks SUCCESS. Verified in a detached worktree; nothing was written to the branch. Nothing unresolved is a blocker. This PR can merge. Deferred findings remain on #764, which this triage updated rather than duplicating.

What was verified at this head

The branch took two master merges since the last triage (8453838, c51ed16), so every previously-fixed finding was re-checked in the committed tree, not inherited from the round-2 record:

  • Round-1 major (write-side sanitize): still fixed. src/core/daemon/status.js:554-555 applies sanitizeLabel(...) ?? 'unknown' / ?? 'all' on write; the discriminating test (sanitizes and clamps hostile dataset and partition labels on write, test file line 138) is in the tree, and CI runs it green, so the fix is still load-bearing.
  • Round-1/2 read-side fixes: still fixed. The fallback phrase at status.js:471 and the partitionsVisited floor at status.js:644 survived the merge, with their tests (lines 325, 483).
  • The One partition's failure no longer ends the maintenance walk (#737) #747 conflict resolution is correct. runtime.js keeps master's startActiveSpan skeleton (with report.totalFailed, partitions_failed / partitions_maintained) and reinstates this PR's three statements: summarizeMaintenanceSkips(report), persist({ maintenance: skips }), and the gated daemon.maintenance_skipped warn line, plus partitions_visited / partitions_skipped beside master's attributes. A new test (a partition this tick failed on is not a skip, line 207) pins the composed behavior: failed partitions stay off this surface, exactly as both PRs' docs said they should. 14 tests total in test/core/status-maintenance-skips.test.js.
  • The renumbering to LLP 0228 is almost fully consistent. Master claimed 0224, the doc is now llp/0228-maintenance-skips-are-a-standing-surface.decision.md, every code @ref says 0228, all three anchors the refs target exist in the doc as HTML anchors, and LLP 0217's Extended-by: entry points at 0228. The one slip is below.
  • Diff vs master is exactly the intended 8 files; no em dashes anywhere in it; the worktree was clean.

Deferred, all non-blocking (on #764)

The stale PR body

The body still says the doc is LLP 0224 and describes #747 as "not merged, not depended on" with a proposed conflict resolution. Judged a preference, not a blocker: the body was accurate when written, the resolution it proposed is exactly what the merge commits executed (verified above), the tree is what merges, and this note is the on-thread correction. For any future reader of the body: read LLP 0228 wherever it says 0224, and the "#747 interaction" section is history that has since played out as described.

Follow-ups: #764 (open, neutral:fix).

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 14, 2026
The forward-ref was written when this document was LLP 0224, and the
0224 to 0226 to 0228 renumbering left the link one step behind. 0226 on
master is the prune decision, so the link resolved to the wrong document.
@philcunliffe philcunliffe removed the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 15, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral triage - re-triage after one doc commit - ship

Head triaged: ca4510c8f9c554b104d21fc7ae172f24723e8735. All 9 checks SUCCESS. Verified in a detached worktree; nothing was written to the branch. The clearance from the c51ed16 triage stands. This PR can merge.

The delta

git diff c51ed16..ca4510c is exactly one line in llp/0218-compaction-failed-attempt-reported.decision.md: the Extended-by: entry that read [LLP 0226](./0226-maintenance-skips-are-a-standing-surface.decision.md) (a dangling link; 0226 is the prune decision) now reads [LLP 0228](./0228-maintenance-skips-are-a-standing-surface.decision.md). No other file, no code.

Verified at this head

One new non-blocking finding (on #764 as item 7)

The rewritten body's "Known, deferred to #764" paragraph still says the llp/0218 entry "still reads LLP 0226 and links a filename that does not exist". True when written, false at this head: ca4510c fixed it. A stale sentence in the body only, no runtime or corpus effect; this note is the on-thread correction.

Follow-ups: #764 (open).

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 15, 2026
Master gained #792, which adds a `proxy trust` section to the same status
render and the same collector LLP 0228's maintenance-skip summary lands in.
The two are conceptually independent, so every conflict resolved as a union:

- src/core/commands/status.js: both imports kept
  (`describeMaintenanceSkipReasons` and `ENV_VAR_NAME`).
- src/core/daemon/status.js: one `@import` line carrying both
  `MaintenanceSkip*` and `ProxyTrustReport`, plus 0228's cache-types import.
- src/core/daemon/status.js: `collectHypAwareStatus` returns both
  `maintenance` and `proxyTrust`.
- src/core/daemon/types.d.ts: `HypAwareStatusReport` carries both fields with
  their own doc comments.

The render bodies merged without conflict and stay in separate places:
`proxy trust` after `recent clients`, `maintenance` after `first sync`, each
still gated on its own condition, neither swallowing the other's spacing.
#792's tri-state (`null` when a probe could not run) and its darwin-plus-CA
gate are untouched.

LLP 0228 is unique across origin/master, every remote branch, and
llp/tombstones after the merge.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe philcunliffe removed the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 17, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral triage - re-triage after the #789/#792/#782 master merge - ship

Head triaged: 2d06270d92d89e6e2568991d471ae5ce1668245f, the merge of master (through #789) into this branch. All 9 checks SUCCESS. Verified in a detached worktree; nothing was written to the branch. Nothing unresolved beyond what #764 already tracks. This PR can merge.

The merge, verified rather than trusted

The only overlap with the three new master PRs is #792, which lands a proxy trust section in the same status surface this PR's maintenance section lands in. Every conflict resolved as a pure additive union, confirmed in the tree:

  • src/core/commands/status.js imports both describeMaintenanceSkipReasons and ENV_VAR_NAME; src/core/daemon/status.js carries one @import line with both MaintenanceSkip* and ProxyTrustReport; collectHypAwareStatus returns both maintenance and proxyTrust; HypAwareStatusReport declares both fields.
  • Diff vs origin/master is exactly this PR's intended 8 files, 1229 insertions, 5 deletions, and all 5 deletions are lines extended in place (three import/@import lines, the two Extended-by: headers on LLP 0217/0218). Nothing this PR needs was dropped: runtime.js:711-712 still runs summarizeMaintenanceSkips(report) and persist({ maintenance: skips }) beside master's totalFailed handling, with the gated daemon.maintenance_skipped warn line.
  • Both sections render together. A harness fed one report with a 3-skip maintenance snapshot and a hyp status reports proxy-mode trust and the launchd env (LLP 0237/0239) #792-shaped proxy trust report (keychain trusted, launchd probe null): the text shows proxy trust: after recent clients and maintenance: after the cache block, each with its own spacing, and --json carries maintenance and proxy_trust as independent keys with the tri-state intact ("launchd_env_set": null survives to the consumer).
  • The two @ref anchors arriving from master via hyp status reports proxy-mode trust and the launchd env (LLP 0237/0239) #792, LLP 0237#consequences and LLP 0239#terminals-predating-attach, both resolve (heading slugs ## Consequences and ### Terminals predating attach); test/core/llp-ref-hygiene.test.js passes 11/11.
  • LLP 0228 is unique after the merge: no duplicate number inside this tree (including llp/tombstones/), none on origin/master, and no other ref anywhere carries an 0228 doc. The Extended-by: forward-refs to 0228 on LLP 0217 and 0218 survived, as did every code @ref to 0228 and its three anchors.
  • Fresh npm install: npm test 4196 pass / 0 fail / 1 skipped, npm run typecheck clean, test/core/status-maintenance-skips.test.js 14/14. No em dashes in the diff vs master.

Deferred

Unchanged and already on #764 (items 1-4 and 7); nothing new to add. Both prior triage markers and the Closes #742 trailer are intact in the body, which now carries the marker for this head.

Follow-ups: #764 (open).

# Conflicts:
#	src/core/commands/status.js
@philcunliffe philcunliffe removed the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 17, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor Author

neutral triage - re-triage after the CI-refresh master merges - ship

Head triaged: 219a9099cb1b48bbcb8f9805ff7df07c722d68b4. All checks green at this head. Verified in a detached worktree; nothing was written to the branch. Nothing unresolved is a blocker. This PR can merge. Deferred findings remain on #764; nothing new to file, so no duplicate issue was created.

The delta since the last triage (2d06270)

Two merge commits only (310d9bc bringing master through v1.23.0 / #797 / #794 / #553 / #680, and 219a909 bringing master through #821). An interdiff of the PR's 8 files between the two heads shows zero change to this PR's own content: src/core/daemon/runtime.js, test/core/status-maintenance-skips.test.js, and all three LLP docs are byte-identical across the merges, and the only movement in src/core/commands/status.js, src/core/daemon/status.js, and src/core/daemon/types.d.ts is master's own work (probe-less client attachable, upstream-name sanitize-and-cap), all orthogonal to the maintenance surface. Diff vs origin/master (a14246d) is still exactly this PR's intended 8 files, 1229 insertions, 5 deletions.

Verified at this head, in the tree

  • Round-1 major (write-side sanitize): still fixed. src/core/daemon/status.js:628-629 applies sanitizeLabel(...) ?? 'unknown' / ?? 'all' on write; the discriminating test (file line 138) is present and green.
  • The tick still persists the report. runtime.js:711-712 runs summarizeMaintenanceSkips(report) and persist({ maintenance: skips }) beside master's totalFailed handling, with the gated daemon.maintenance_skipped warn line (counts plus sanitized worst only).
  • Both caps and the read-side cleaning survived. MAX_SKIPPED_PARTITIONS_REPORTED enforced on write (status.js:614) and read (:688); read side sanitizes dataset, partition, and failedAt (:695-699); the ... and N more (hyp query maintain --dry-run lists them all) tail renders at commands/status.js:563. Master's new upstream-name path uses the same sanitize-on-read pattern, so the two do not diverge in kind.
  • Fresh install: npm test 4271 pass / 0 fail / 1 skipped; npm run typecheck clean; status-maintenance-skips 14/14.

Residual findings, classified (all preferences, all already on #764)

  1. LLP 0228 privacy sentence (llp/0228:102-104, Follow-ups from PR #759 triage: LLP 0224 factual correction, foreign-file reason floor, three nits #764 item 1): still says nothing row-derived is on the path; false, but non-behavioural, and the load-bearing knowledge is duplicated in the code comment at the sanitize site. Doc inaccuracy: preference.
  2. Foreign-file reason floor (status.js:545 fallback phrase vs maintenanceSkipsFromStatus, Follow-ups from PR #759 triage: LLP 0224 factual correction, foreign-file reason floor, three nits #764 item 2): reachable only from a hand-edited or foreign status.json; worst outcome is a wrong phrase above a correct, sanitized list. Preference.
  3. Over-claiming test comment (test file line 317, Follow-ups from PR #759 triage: LLP 0224 factual correction, foreign-file reason floor, three nits #764 item 3): comment only. Preference.
  4. failedAt unsanitized on write (status.js:636-637, Follow-ups from PR #759 triage: LLP 0224 factual correction, foreign-file reason floor, three nits #764 item 4): the only item that brushes sanitizing, so it got the closest look. Our code writes only ISO timestamps there; the value never reaches the daemon log line (counts plus worst only, both sanitized); the read side sanitizes it at :699 before any render. Exposure is raw bytes inside a local status.json, requiring an attacker who can already write local cursor files, i.e. the same trust domain as the status file itself. Not a production defect: preference.
  5. Item 5 is moot (One partition's failure no longer ends the maintenance walk (#737) #747 merged), item 6 is fixed in-branch (ca4510c), and item 7 (the stale body sentence) is resolved: the body now records the ca4510c fix in past tense.

Nothing found at this head is untracked, so #764 is the tracking issue for everything residual.

@philcunliffe philcunliffe added neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) and removed neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) labels Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Daemon maintenance skip reasons are visible only in spans, never in a standing daemon surface

1 participant